-
Notifications
You must be signed in to change notification settings - Fork 10.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(gatsby): Set up Fast Refresh #29588
Conversation
3b9df5b
to
a9b700e
Compare
…me errors, change graphql errors emitting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't go through the whole code but this is what I found with a rough glimpse
packages/gatsby/cache-dir/fast-refresh-overlay/components/code-frame.js
Outdated
Show resolved
Hide resolved
React.useEffect(() => { | ||
async function fetchData() { | ||
const res = await fetch(url) | ||
const json = await res.json() | ||
const decoded = prettifyStack(json.codeFrame) | ||
const { sourcePosition, sourceContent } = json | ||
setResponse({ | ||
decoded, | ||
sourceContent, | ||
sourcePosition, | ||
}) | ||
} | ||
fetchData() | ||
}, []) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you probably want to use abortcontroller here so the fetch aborts when we unmount.
A dirty fix could be to add a is Mounted ref and not set the response when it's false.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried it with this and it completely broke the current working behavior:
const controller = new AbortController()
const { signal } = controller
React.useEffect(() => {
async function fetchData() {
const res = await fetch(url, { signal })
const json = await res.json()
const decoded = prettifyStack(json.codeFrame)
const { sourcePosition, sourceContent } = json
setResponse({
decoded,
sourceContent,
sourcePosition,
})
}
fetchData()
return () => {
controller.abort()
}
}, [])
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:o you need to ue refs but let's do it after :)
return null | ||
}) | ||
.filter(Boolean) | ||
errorsToDisplay = errors.flatMap(e => e).filter(Boolean) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not necessary to do this handling here, I grab all info in the graphql-error component itself.
Also, we can use flatMap
because we're now at Node 12.13 👍 and that's a Node 11 feature.
}) { | ||
const [isOpen, setIsOpen] = React.useState(open) | ||
const [prevIsOpen, setPrevIsOpen] = React.useState(open) | ||
const id = useId(`accordion-item`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need a stable id here (for both SSR and client for re-hydration) so that's why this helper exists
} | ||
|
||
render() { | ||
return this.state.hasError ? null : this.props.children | ||
// Without this check => possible infinite loop | ||
return this.state.error && this.props.hasErrors ? null : this.props.children |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Necessary to also have this.state.error
so that the background doesn't vanish
<Backdrop /> | ||
<div | ||
data-gatsby-overlay="root" | ||
role="alertdialog" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Purposefully chose alertdialog here and not dialog.
The difference with regular dialogs is that the alertdialog role should only used when an alert, error, or warning occurs. In other words, when a dialog's information and controls require the user's immediate attention alertdialog should be used instead of dialog. It is up to the developer to make this distinction though.
This fits our use case here
<h1 id="gatsby-overlay-labelledby">Failed to compile</h1> | ||
<span>{file}</span> | ||
</div> | ||
<HeaderOpenClose open={() => openInEditor(file, 1)} dismiss={false} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a follow up: we should put this openInEditor on the same place as runtime errors so people know where to look.
Description
This PR improves our current Fast Refresh integration. Instead of completely disabling the
overlay
for theReactRefreshWebpackPlugin
we now pass a custommodule
and rely on thewhm
integration. We also needed to vendorwebpack-hot-middleware
.List of changes made:
mitt
we push to a queue inwindow._gatsbyEvents
because sometimes we need actions before React rehydrated andmitt
was too late for that thencompilation.codeGenerationResults
inside start-server.tsfast-refresh-overlay/index.js
to use a reducer for its state logicfast-refresh-module
for themodule
option and expose all expected optionsDocumentation
In case we have pages where we show an error overlay screenshot, those would need to be updated.
Testing
In case you want to test it, here's a
src/pages/index.js
file I used for development.Related Issues
Initial integration: #26664
[ch25595]
[ch25647]
[ch19583]
[ch25648]